[Feature, n/a] updated dependencies #151

davesag · 2017-11-16T06:59:34Z

I have upgraded a number of dependencies to remove most of the deprecation warnings.

upgraded a number of dependencies to remove some deprecation warnings
updated node version for circle to test under Node 10
updated circleci config to version 2 format
removed the --fix option from the default lint command. (use npm run lint -- --fix to fix.)
cleaned up all linting errors

tested under Node 10.1 and Node LTS

…lparse

davesag · 2017-11-16T07:25:08Z

tests all pass but Circle timed out for some reason

davesag · 2017-11-16T07:48:30Z

Again, tests are passing but the final step is timing out.

cgewecke · 2017-11-16T22:12:57Z

@davesag Thanks so much for combing through the code like this, lot's of good stuff in this PR. Reviewing....

cgewecke · 2017-11-16T22:43:41Z

lib/app.js

+    //       allow
+    //       const { coverage } = this.coverage;
+    // ref rule: object-curly-newline
+    const {


Yes! Add that rule if you'd like. Sound's reasonable. Also this rewrite of makeKeysRelative is nice.

I'll change the rule to "object-curly-newline": ["error", { "minProperties": 3 }],

cgewecke · 2017-11-16T22:58:49Z

package.json

    "shelljs": "^0.7.4",
    "sol-explore": "^1.6.2",
-    "solidity-parser-sc": "0.4.1",
+    "solparse": "^1.4.0",


solparse is great - it's a more robust implementation than sc and is very well maintained. And we have a friendly collaboration with them because we both helped with the upstream consensys parser.

That said, there are benefits to self-publishing sc - for example it requires less coordination and we can often get away with much coarser parsing than they can - they're supporting a linter. If this was painful to maintain I would solparse in a second but atm it's not and it's convenient to be able to quickly add the occasional over-permissive rule to stop solidity-coverage from crashing.

Also we're actively trying to re-design ourselves around the solc ast in #111 and #150.

Open to persuasion here but I'd rather not do this in this PR.

solidity-parser-sc is terribly out of date dependency-wise vs solparser (which I recently contributed some changes to). I think it makes sense to consolidate dependencies wherever possible. The alternative is fixing all the out of date dependencies in solidity-parser-sc as well and effectively keeping them in sync. thoughts?

Eminently reasonable. In fact we went through a long period of trying to unify under a single parser and solparse is definitely the best thing out there right now. Thanks for contributing to it - fantastic.

On reflection, I'd rather preserve the flexibility/control that self-publishing provides since we're trying to move to a different AST and it's important to keep our parsing super consistent during that process so we know what behavior we're duplicating.

This change can't go in this PR, I'm really sorry. It's too consequential. I think it's ok for the time being if sc has out-of-date deps, and we should revisit using solparse if it looks like we'll be stuck in the current design for a while or the parser breaks.

@cgewecke where is the repo for solidity-parser-sc? If you are really wedded to that then I'd like to fix the dependencies.

cgewecke · 2017-11-16T22:59:08Z

test/cli/command-options.js

+  topics: ['34b35f4b1a8c3eb2caa69f05fb5aadc827cedd2d8eb3bb3623b6c4bba3baec17'],
+  // eslint-disable-next-line max-len
+  data: '00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000003a2f55736572732f757365722f53697465732f73632d666f726b732f6d657461636f696e2f636f6e7472616374732f4d657461436f696e2e736f6c000000000000',
+};



Yes, thank you.

cgewecke · 2017-11-16T22:59:31Z

test/util/vm.js

@@ -11,7 +11,7 @@ const coder = require('web3/lib/solidity/coder.js');

 // Don't use this address for anything, obviously!
 const secretKey = 'e81cb653c260ee12c72ec8750e6bfd8a4dc2c3d7e3ede68dd2f150d0a67263d8';
-const accountAddress = new Buffer('7caf6f9bc8b3ba5c7824f934c826bd6dc38c8467', 'hex');
+const accountAddress = Buffer.from('7caf6f9bc8b3ba5c7824f934c826bd6dc38c8467', 'hex');



cgewecke · 2017-11-16T23:00:59Z

package.json

    "request": "^2.81.0",
-    "solc": "0.4.17",
-    "truffle": "4.0.0-beta.2"
+    "solc": "^0.4.17",


Let's leave this pinned and change it when we explicitly decide to test above. Currently we check behavior above this point using the experimental pragma.

cgewecke · 2017-11-16T23:03:05Z

package.json

-    "mocha": "^3.1.0",
+    "ethereumjs-vm": "git+https://github.com/sc-forks/ethereumjs-vm-sc.git#6be86e6f12634b711423203e6744cafe07c38e69",
+    "merkle-patricia-tree": "~2.2.0",
+    "mocha": "^4.0.1",


Can we bump this back down and see if that fixes the Circle hang?

It's the Mocha change that fixes a lot of the dependency issues. The tests pass on CircleCI so I'm not sure why it would be the culprit.

Hmmm, yes and you're getting it to pass locally. I wonder what the difference is. Different Istanbul locally?

I've worked it out. For CircleCI and Mocha4 need to pass in --exit to the test command. Done that and now it exits properly without timing out.

cgewecke · 2017-11-16T23:05:26Z

package.json

@@ -1,13 +1,17 @@
 {
  "name": "solidity-coverage",
-  "version": "0.4.0",
+  "version": "0.4.1",
  "description": "",


Let's leave this at 0.4.0 and let npm do it when we run npm version patch and publish.

cgewecke · 2017-11-16T23:06:18Z

docs/testrpc-sc.md

-
-
-
-


See parser upgrade notes below.

davesag · 2017-11-17T01:14:19Z

@cgewecke Hi Christopher, I've pushed a number of changes based on your suggestions above. I'm open to fixing the dependency issues in solidity-parser-sc but I'd prefer not to honestly as I've kind of run over time. I really only dived in and fixed this stuff in order to advance a project I am working on. Happy to discuss however.

Also I didn't downgrade Mocha as that's where a number of the dependency issues were coming from. As mentioned above I don't believe Mocha would be the reason CircleCI is timing out. It appears to be timing out on the connection to codecov.io. But then it's worth noting that CircleCI was having huge issues across the board yesterday, so maybe it's just a temporary issue. I'm more of a TravisCI person myself and not too familiar to CircleCI.

davesag · 2017-11-17T01:26:32Z

also worth noting I checked the full npm run test-cov on my local

Statements   : 98.84% ( 341/345 )
Branches     : 88.24% ( 120/136 )
Functions    : 100% ( 51/51 )
Lines        : 98.81% ( 333/337 )

cgewecke · 2017-11-17T01:50:29Z

.eslintrc

@@ -33,7 +33,7 @@
      "arraysInObjects": false
    }],
    "no-unused-expressions": ["error", { "allowShortCircuit": true, "allowTernary": true }],
-    "object-curly-newline": ["error", { "minProperties": 1 }],
+    "object-curly-newline": ["error", { "minProperties": 3 }],
    "object-property-newline": ["error"],


@area If you have a sec, could you give your opinion about this rule change? It reformats the code pretty extensively.

@cgewecke and @area do you have a slack or IRC or something? gitter perhaps?

I don't mind 😄. No, we don't have any of those tools set up. I suppose something like that might be useful though.

davesag · 2017-11-17T02:29:23Z

Yeah! That fixed it.

codecov-io · 2017-11-17T02:29:27Z

Codecov Report

Merging #151 into master will increase coverage by 0.23%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   95.38%   95.61%   +0.23%     
==========================================
  Files           6        6              
  Lines         368      365       -3     
  Branches       84       84              
==========================================
- Hits          351      349       -2     
+ Misses         17       16       -1

Impacted Files	Coverage Δ
lib/preprocessor.js	`95.34% <100%> (-0.11%)`	⬇️
lib/instrumenter.js	`96.87% <100%> (+1.42%)`	⬆️
lib/instrumentSolidity.js	`100% <100%> (ø)`	⬆️
lib/coverageMap.js	`100% <100%> (ø)`	⬆️
lib/parse.js	`91.17% <66.66%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 031ba15...880ead1. Read the comment docs.

davesag · 2017-11-17T02:30:53Z

💩 test coverage dropped by 0.01%. grr

davesag · 2017-11-17T05:52:50Z

😃 test coverage up by 1.33%

…run npm run lint -- --fix

aleybovich · 2018-02-07T23:12:16Z

Are you guys planning to merge this PR? It looks very useful as it updates lots of dependencies

matt-lough · 2018-04-12T04:13:18Z

lib/parse.js

+const ASSERT = 'assert';
+const IDENTIFIER = 'Identifier';
+const EVENT_DEF = { type: 'eventDefinition' };
+const LEFT_BRACE = '{';


NoahZinsmeister · 2018-11-29T15:44:55Z

@cgewecke seeing as solidity-coverage is broken with Solidity v0.5.0, is there a migration path planned to merge this PR and migrate to solparse or solidity-antlr4 that would address these issues? See #258 for an example.

cgewecke · 2018-11-29T21:30:04Z

@NoahZinsmeister Thanks for pinging - I will try to fix the calldata problem in #258 and the assembly issue in #287 ASAP. I'm not sure any of the JS parsers are fully 0.5.0 compliant at the moment unfortunately.

NoahZinsmeister · 2018-11-30T00:14:31Z

Sure thing @cgewecke! Let me know if there's anything I can do to help :)

cgewecke · 2019-09-09T08:14:52Z

Closing for house-keeping.

davesag added 4 commits November 16, 2017 12:18

updated dependencies and fixed linting issues

864f546

updated node version to latest LTS version

8344952

removed reference to solidity-parser-sc as it's been replaced with so…

540d9f4

…lparse

updated changelog

2de7242

minor change to trigger another build

ae79195

cgewecke reviewed Nov 16, 2017

View reviewed changes

alts based on feedback to PR

58f9e45

restored notes to docs with minor corrections to spelling and formatting

f767479

cgewecke reviewed Nov 17, 2017

View reviewed changes

added --exit to mocha options to try and fix the CircleCI timout issue

e61b6cc

improved test coverage stats

eeb8577

davesag added 3 commits November 17, 2017 11:32

ignore coverage folder

c1decf2

fixed linting

f3c079d

remove --fix from lint script as this confuses CircleCI. To auto fix …

b02b10a

…run npm run lint -- --fix

davesag added 2 commits November 17, 2017 11:34

added linting to circleCI config

c938eb7

reverted solparse back to solidity-parser-sc

ec4c355

davesag mentioned this pull request Nov 20, 2017

Instrumentation appears to be breaking constants #153

Closed

davesag added 2 commits November 20, 2017 10:09

n/a merged in changes from release 1.4.1

a20cd9a

n/a merged in changes from 1.4.2

d09a460

merged in changes from 1.4.9

f7157ce

matt-lough reviewed Apr 12, 2018

View reviewed changes

rebased from master, updated deps, updated circleci config, linted

880ead1

davesag mentioned this pull request May 20, 2018

Deprecation waarnings #230

Closed

cgewecke closed this Sep 9, 2019

[Feature, n/a] updated dependencies #151

[Feature, n/a] updated dependencies #151

Conversation

davesag commented Nov 16, 2017 • edited Loading

davesag commented Nov 16, 2017 • edited Loading

davesag commented Nov 16, 2017

cgewecke commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesag commented Nov 17, 2017

davesag commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesag commented Nov 17, 2017

codecov-io commented Nov 17, 2017 • edited Loading

Codecov Report

davesag commented Nov 17, 2017

davesag commented Nov 17, 2017

aleybovich commented Feb 7, 2018

Choose a reason for hiding this comment

NoahZinsmeister commented Nov 29, 2018

cgewecke commented Nov 29, 2018

NoahZinsmeister commented Nov 30, 2018

cgewecke commented Sep 9, 2019

davesag commented Nov 16, 2017 •

edited

Loading

davesag commented Nov 16, 2017 •

edited

Loading

codecov-io commented Nov 17, 2017 •

edited

Loading